iOS: add CodePushBinaryDiffPatcher to apply bsdiff patches - #59
Conversation
ofalvai
changed the title
7912ae8 to
ca765df
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new patcher currently emits misleading hash-mismatch errors when hashing fails (e.g., unreadable/missing files), obscuring the real I/O failure cause.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds an iOS-native binary diff patcher to apply bsdiff patches described by CodePushDiffManifest, aligning iOS with the existing Android diff/patch flow and hardening path handling via resolvePath(_:withinFolder:).
Changes:
- Introduces
CodePushBinaryDiffPatcher(Obj-C) to validate hashes, resolve manifest-supplied paths safely, and applybsdiffpatches viabspatch_bridge. - Adds Swift XCTest coverage for happy-path patching, hash mismatches, unsupported algorithms, path traversal, and symlink-escape attempts.
- Wires the new source + tests into the Xcode project and test bridging header.
File summaries
| File | Description |
|---|---|
| ios/CodePushTests/CodePushTests-Bridging-Header.h | Exposes the new Obj-C patcher to Swift tests. |
| ios/CodePushTests/CodePushBinaryDiffPatcherTests.swift | Adds test coverage for binary patch application and path/symlink safety. |
| ios/CodePush/CodePushBinaryDiffPatcher.m | Implements patch application, path resolution, and hash verification. |
| ios/CodePush/CodePushBinaryDiffPatcher.h | Declares the patcher API and Swift import name. |
| ios/CodePush.xcodeproj/project.pbxproj | Registers new sources/headers and the new test file in build phases. |
Review details
Suppressed comments (1)
ios/CodePush/CodePushBinaryDiffPatcher.m:98
- If hashing the newly patched file fails,
newFileHashcan be nil and the code reports a confusing "targetHash mismatch ... got (null)" instead of the actual hashing/read error. Handling the nil case separately provides a clearer failure reason.
NSString *newFileHash = CodePushSha256HexForFile(newFile, &hashError);
if (!newFileHash || ![newFileHash isEqualToString:entry.targetHash]) {
if (error) *error = patchApplyError(relativePath, [NSString stringWithFormat:@"targetHash mismatch: expected %@, got %@", entry.targetHash, newFileHash]);
return NO;
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| NSError *hashError = nil; | ||
| NSString *oldFileHash = CodePushSha256HexForFile(oldFile, &hashError); | ||
| if (!oldFileHash || ![oldFileHash isEqualToString:entry.baseHash]) { | ||
| if (error) *error = patchApplyError(relativePath, [NSString stringWithFormat:@"baseHash mismatch: expected %@, got %@", entry.baseHash, oldFileHash]); | ||
| return NO; | ||
| } |
ca765df to
1f18747
Compare
1f18747 to
4cd11f4
Compare
miklosboros
left a comment
miklosboros
left a comment
There was a problem hiding this comment.
Two notes from reviewing this against ios-diff-manifest-parsing and Android's BinaryDiffPatcher.kt. The patcher itself reads well — hash before and after, the two-pass validation, and the bridge's own cleanup mean a failed patch can't leave a wrong-but-plausible file behind, and the error messages carry more detail than Android's.
| 1993F82DF6330426AC5CBC2E /* CodePushBinaryDiffPatcher.m in Sources */ = {isa = PBXBuildFile; fileRef = 9414C770C70E1B1319F24B12 /* CodePushBinaryDiffPatcher.m */; }; | ||
| F41E2EFAD0D322F7D14D7125 /* CodePushBinaryDiffPatcher.m in Sources */ = {isa = PBXBuildFile; fileRef = 9414C770C70E1B1319F24B12 /* CodePushBinaryDiffPatcher.m */; }; | ||
| D5BC8B1BF7D2DA03BB888FCC /* CodePushBinaryDiffPatcher.m in Sources */ = {isa = PBXBuildFile; fileRef = 9414C770C70E1B1319F24B12 /* CodePushBinaryDiffPatcher.m */; }; |
There was a problem hiding this comment.
These three entries add CodePushBinaryDiffPatcher.m to CodePush, CodePush-tvOS and CodePushTests — but shared/diffpatch/bspatch_bridge.c (and the hdiffpatch sources) are not in the tvOS target, and the patcher's codepush_bspatch_apply call at CodePushBinaryDiffPatcher.m:89 is its only caller. I parsed both trees to confirm:
BASE (#58) tvOS: bspatch_bridge.c=no patcher=no Sha256=no UpdateUtils=YES
PR#59 tvOS: bspatch_bridge.c=no patcher=YES Sha256=no UpdateUtils=YES
So _codepush_bspatch_apply becomes unresolved in CodePush-tvOS, and it wasn't before this PR.
Two things keep this from being a build break, and I want to be precise about them rather than overstate it: CodePush-tvOS is a static library, so unresolved symbols are legal in the archive and xcodebuild will not fail on that target. And CocoaPods consumers are unaffected — the podspec compiles shared/diffpatch/*.c and the hdiffpatch sources for tvOS as well. It surfaces at app link time only for someone integrating tvOS manually from the .xcodeproj.
Separately and pre-existing, so not yours to fix here: _CodePushSha256HexForFile is also unresolved in that target — CodePushUpdateUtils.m already referenced it on the base branch with CodePushSha256.m absent. The patcher just becomes a second caller.
Either drop the patcher from CodePush-tvOS, or add bspatch_bridge.c + the hdiffpatch sources (and CodePushSha256.m) to it.
There was a problem hiding this comment.
This tvOS target frequently gets flagged, and we should clean it up one day, I just want to better understand what was its original purpose. It's been broken for years, we don't really use it in CI anyway, and it doesn't get distributed to users (Cocoapods is the distribution mechanism). I think it's fine for now.
| static NSString *resolveWithin(NSString *base, NSString *path, NSString *manifestEntry, NSError **error) | ||
| { | ||
| NSString *resolved = [CodePushDiffManifest resolvePath:path withinFolder:base]; | ||
| if (resolved == nil) { | ||
| if (error) *error = patchApplyError(manifestEntry, @"path escapes expected directory"); | ||
| } | ||
| return resolved; | ||
| } |
There was a problem hiding this comment.
This maps every nil from resolvePath:withinFolder: to "path escapes expected directory", but #58's resolver returns nil for at least four distinct causes:
- an actual escape
- an empty
relativePath - a path the file system can't represent (embedded NUL, or longer than
PATH_MAX) - a base folder that doesn't exist — it's
realpath-based, which iOS: add CodePushDiffManifest for parsing diff manifests #58's owntestResolvePath_missingFolder_isRejectedpins down
Two of those are manifest-triggerable: patchedFiles: {"": {...}}, or a key containing a NUL, both report a path escape for what is only a malformed manifest.
Worth noting this is an iOS-only divergence. Android's resolveWithin uses File.canonicalFile, which doesn't require the path to exist, so a missing currentPackageFolder there surfaces as a genuine read failure out of sha256Hex instead. Same manifest, same condition, very different diagnosis — and "path escapes" is the one that sends whoever reads the log looking for an attack.
Threading the reason out of resolvePath (or at least distinguishing "base folder unusable" and "path not representable" from a real escape) would fix it.
There was a problem hiding this comment.
Decided to fix this by adopting the standard ObjC pattern of a NSError* argument. The caller can log this root cause, and ObjC -> Swift interop transforms this pattern into real exceptions.
PR #58 is updated with the change to resolvePath:withinFolder, and this PR with the changes to the callsites.
e27524d to
3a07cf9
Compare
3a07cf9 to
fc26ab7
Compare
Context
iOS counterpart of #42. This is the diff patching logic that #48 is going to integrate into the main package install flow.
What
CodePushBinaryDiffPatchertakes a manifest (from #58), the current update folder, the unzipped update folder, and the folder where the new update files should live after applying the patches.